Skip to content

479 mirai lockfile@main #1263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 100 commits into from
Sep 23, 2024
Merged

479 mirai lockfile@main #1263

merged 100 commits into from
Sep 23, 2024

Conversation

@gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Jul 12, 2024 •
edited
Loading

#1276

@pawelru @m7pr
This is a mirai alternative. Package seems to address the biggest issues reported during a research:

  • mirai has a native support of ExtendedTask
  • mirai is not being killed when runApp is executed (like callr does)
  • mirai by default opens a deamon in parallel R session without a need to handle the future::plan.
  • mirai has only one dependency in the whole dependency tree.

Disadvantages so far:

How does it work:

  • lockfile creation is invoked in init before application starts. This prevents to start the process each time when a new shiny session starts. Process is invoked as a promise and eventually teal_app.lock will be created
  • When shiny session starts download lockfile button is hidden by default. If promise is eventually resolved and lockfile is created then download button is shown.
  • alternatively, app developer can pre-compute lockfile and provide its path in teal.renv.lockfile option. In such case renv::snapshot will be skipped and user lockfile will be used in an app.

Logs and notifications

Logs are printed for app developer while notifications are presented to the app user:

  1. When app uses precomputed file:
  • log in init: Lockfile set using option "teal.renv.lockfile" - skipping automatic creation.
  • no notification to the app user.
  1. When app automatically determines snapshot:
  • log in init: Lockfile creation started based on { getwd() }.
  • log If lockfile created: Lockfile {path} containing { n-pkgs } packages created{ with errors or warnings }.
  • notification if lockfile created: Lockfile available to download
  • log if lockfile not created: Lockfile creation failed.
  • notification if lockfile not created: Lockfile creation failed.

m7pr and others added 30 commits July 1, 2024 12:13
@m7pr
substitute future::promises lockfile creation with callr::r_bg
249390c
@m7pr
fix tests
d1efcc7
@m7pr
cleanup the old approach
9bd9944
@github-actions
[skip style] [skip vbump] Restyle files
5911bd3
@m7pr
do not assign renv output to anything
e46337c
@m7pr
no need to capture renv::snapshot output as nothing gets printed out …
f0ee2fd 
…of callr
@m7pr
Merge branch '479_callr_lockfile@main' of https://github.com/insights…
b90e1ef 
…engineering/teal into 479_callr_lockfile@main
@m7pr
pass options and renv objects
3650a9d
@github-actions
[skip style] [skip vbump] Restyle files
0689000
@m7pr
add comments
b40417e 
Signed-off-by: Marcin <[email protected]>
@github-actions
[skip style] [skip vbump] Restyle files
f4b51d6
@m7pr
bring back logging
e04fff1
@github-actions
[skip style] [skip vbump] Restyle files
13d965a
@m7pr
prefixes
23df7ca
@m7pr
merge
746690f 
Merge branch '479_callr_lockfile@main' of https://github.com/insightsengineering/teal into 479_callr_lockfile@main

# Conflicts:
#	R/teal_lockfile.R
@github-actions
[skip style] [skip vbump] Restyle files
b717558
@m7pr @pawelru
Update R/teal_lockfile.R
9208909 
Co-authored-by: Pawel Rucki <[email protected]>
Signed-off-by: Marcin <[email protected]>
@m7pr @pawelru
Update tests/testthat/test-utils.R
f7fbc87 
Co-authored-by: Pawel Rucki <[email protected]>
Signed-off-by: Marcin <[email protected]>
@m7pr
Merge branch 'main' into 479_callr_lockfile@main
03c2dce
@m7pr
test for a lockfile in init
d400570
@m7pr
less capture.output is needed
e7c7c7b
@m7pr
expose env = NULL and unify documentation
6b1c0df
@m7pr
bring create_renv_lockfile_2 for testing
0114333
@github-actions
[skip style] [skip vbump] Restyle files
58a9e8d
@m7pr
append the name to the counter for the test function
2632e5c
@m7pr
merge
db24fbd 
Merge branch '479_callr_lockfile@main' of https://github.com/insightsengineering/teal into 479_callr_lockfile@main

# Conflicts:
#	R/teal_lockfile.R
@m7pr
store r_bg process to a variable to prevent from killing the process
027faee
@m7pr
prototype of the reactivePoll
4467cd2
@github-actions
[skip style] [skip vbump] Restyle files
86403e8
@m7pr
styler check
d6a82e4
@gogonzo
Copy link
Contributor Author

gogonzo commented Sep 19, 2024

@pawelru this is ready ⭐

pawelru
pawelru reviewed Sep 19, 2024
View reviewed changes
pawelru
pawelru reviewed Sep 19, 2024
View reviewed changes
m7pr
m7pr reviewed Sep 19, 2024
View reviewed changes
gogonzo
gogonzo commented Sep 19, 2024
View reviewed changes
Copy link
Contributor Author

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments

pawelru
pawelru reviewed Sep 19, 2024
View reviewed changes
pawelru
pawelru reviewed Sep 19, 2024
View reviewed changes
gogonzo
gogonzo commented Sep 19, 2024
View reviewed changes
m7pr
m7pr approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@m7pr m7pr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pawelru since @gogonzo is the author, he can not approve the PR. I implemented all the changes we discussed and I am stating this is solid. Any final words?

pawelru
pawelru reviewed Sep 20, 2024
View reviewed changes
pawelru
pawelru approved these changes Sep 20, 2024
View reviewed changes
Copy link
Contributor

@pawelru pawelru left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 it took us a while to get there :)

@m7pr
Copy link
Contributor

m7pr commented Sep 20, 2024

@gogonzo you can do the honors of merging this in : ) great colab team!

@gogonzo gogonzo merged commit 1b4bb50 into main Sep 23, 2024
28 checks passed
@gogonzo gogonzo deleted the 479_mirai_lockfile@main branch September 23, 2024 04:47
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@m7pr m7pr m7pr approved these changes

@pawelru pawelru pawelru approved these changes

+1 more reviewer

@chlebowa chlebowa chlebowa left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

core

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@gogonzo @m7pr @pawelru @chlebowa